Implement Iterator for Bound<'py, PySequence> #3927
Conversation
|
Thanks for proposing this! I think it's definitely an open design question. There's some possibility already for the set and frozenset iterators to panic in the event of a poorly-implented subclass. That said, I think poorly-implemented sequences seems more likely. Is that a problem? I'm not sure. I'd be interested to know what was the original motivation. Was it completeness? Do you use this one downstream? Is this noticeably faster than going via While it's true that this could be a good time to add it, I'm also tempted to argue the opposite, and that (where possible) we should be aiming to keep the differences between the Bound and GIL Ref APIs as small as possible. I think adding this in a future PyO3 would be breaking but might not be that hard to fixup in isolation, so I'm not super worried if this doesn't make it in 0.21. |
Yeah, me neither.
Pretty much this - I was looking through the pyo3 codebase for any uses of
No.
I haven't tested this.
Fair.
Avoiding the need for a breaking change was the main motivation for suggesting it for 0.21. |
|
I've been thinking about this more. Maybe the right way forward here is to just make the iteration fallible? It would at least avoid the need for the A related question would be whether any other types deserve iterators too. |
This allows using a `Bound<'py, PySequence>` directly in a for loop.
LilyFoote
force-pushed
the
pysequence-iter
branch
from
cfed22f
to
1e6fcf8
Compare
There was a problem hiding this comment.
Sorry for the long pause here; I'd missed that you pushed the fallible iteration.
In light of #4553, this might now have more space in the API as a clearly separate method from .try_iter().
That said, I'm a little uneasy about the move away from Python iteration here, and the subtleties of the breaking change. e.g. what happens if the sequence implements __iter__ and does something a bit... exotic?
Perhaps let's revisit in 0.24 when users should have migrated off .iter(), so it's less likely to be an in-place breaking change.
| unsafe fn get_item(&self, index: usize) -> PyResult<Bound<'py, PyAny>> { | ||
| self.sequence.get_item(index) | ||
| } |
There was a problem hiding this comment.
| unsafe fn get_item(&self, index: usize) -> PyResult<Bound<'py, PyAny>> { | |
| self.sequence.get_item(index) | |
| } | |
| fn get_item(&self, index: usize) -> PyResult<Bound<'py, PyAny>> { | |
| self.sequence.get_item(index) | |
| } |
When working on #3923 I noticed that we didn't have an implementation of
IteratorforPySequence. I'm not 100% certain this is the right thing to add, since it hides the fallibility oflenandget_itemwith.expectcalls. But a properly implemented PythonSequenceshouldn't raise in these, so maybe the ergonomics justify allowing panics here?If we want this, I think now would be a good time to add it, since the changes to error handling it introduces would be straightforward as part of the general
Boundmigration.